Repository navigation
fix(auth): isolate API keys to exact credentials and organizations - #7461
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — API-key credential-boundary hardening (server-only).
Verified fail-closed terminal API-key auth, owner-type/shape validation, exact-credential authorization via authWorkosUserId, selector hardening with encodeURIComponent, fresh-every-request ban checks, and the attachIdentityId concurrency capture — all sound and well-tested. This is a server-only change; no schema/docs/changeset surface touched, so protocol-governance gates do not apply.
Medium findings
- server/src/middleware/api-key-format.ts — validateWorkOSApiKey's createValidation runs on the module-global WorkOS client with no timeout/maxRetries on the now-terminal, uncached, every-request path, unlike the sibling fail-fast clients. Worth a follow-up but non-blocking.
Decision-table walk: no critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk is true but the two matching files are (modified) with only a single medium finding — row 5 requires a medium finding on a modified high-risk file, which technically applies... however the sole medium is on api-key-format.ts (middleware), not on the modified high-risk-path files (routes/api-keys.ts, routes/helpers/resolve-caller-org.ts). No medium finding lands on a high-risk-path (modified) file, so row 5 does not fire. No data-loss/schema/infra medium (row 4 n/a). No deletions (row 3 n/a). No prior escalation (row 6 n/a). No no-auto-approve team (row 7 n/a). Only 1 medium finding — fewer than 3 (row 8 n/a). Falls through to row 9: approve.
b310bea to
2f058a5
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — WorkOS auth hardening rework (server/platform-only).
No blocking findings. The reviewer traced requireAuth/optionalAuth end-to-end, plus api-keys.ts selector/dot-segment guards and caller-org 401/503 propagation. Error handling is fail-closed and extensively tested (exact-credential authorization; presented bearers never fall back to cookie/remembered/primary org; provider-unavailable → 503, token-invalid → 401). No protocol surface, dist, or schema touched, so no changeset is required.
Gate checks:
- No no-auto-approve team match.
- gated_paths: false → row 2 does not apply.
- high_risk: true, but all reasons are
(modified)server/src/routes/** files with NO medium-or-higher findings → row 3 (deleted) and row 5 (modified + medium) do not fire. - No critical/high/medium findings → rows 1, 4, 8 do not apply.
- Prior decision was
approve, so row 6 (sticky escalate) does not apply.
None of rows 1–8 fired; falls through to row 9 → approve.
2f058a5 to
719b5f8
Compare
423f6d8 to
b1789a6
Compare
719b5f8 to
4f546bf
Compare
The base branch was changed.
4f546bf to
0356871
Compare
7b9876d to
438f952
Compare
438f952 to
3216b0d
Compare
Bind API-key authentication and management to the exact credential and explicit organization. Require direct active membership for management, validate opaque resource identifiers, and keep unfenced key mutations disabled. Buffer inventory until final authority checks pass. Preserve the snapshot/authn-cache parent and add route, concurrency, lifecycle, and PostgreSQL regressions.
6cb4b16 to
2d96ea6
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean auth-hardening pass, no blocking findings.
The reviewer re-read every non-test source file in the diff (auth.ts, api-key-management-db.ts, api-keys.ts, resolve-caller-org.ts, workos-jwt.ts, conformance/token-route.ts, community-mirrors.ts, registry-api.ts, admin.ts) and confirmed fail-closed auth hardening: bounded timeouts on all WorkOS/fetch/DB paths, no pool leak in the fence, bearer credentials terminal with no cookie/dev fallback, and the new resolveCallerOrgId throw handled by all three call sites. No changeset/schema/dist surface touched.
Decision-table walk:
- Row 1 (critical/high): no such findings.
- Row 2 (gated_paths): gated_paths is false — does not fire.
- Row 3 (deleted high-risk): all high-risk reasons are (modified); no deletions.
- Row 4 (medium data-loss/schema/infra): no medium findings.
- Row 5 (modified high-risk + medium): high_risk is true with (modified) files, but there are zero medium findings.
- Row 6 (sticky escalate): prior decision was approve, not escalate.
- Row 7 (no-auto-approve team): no team match.
- Row 8 (≥3 medium): zero medium findings.
- Row 9: approve.
high_risk is true only via (modified) matches on server/src/routes/**, and the reviewer found no medium-or-higher concerns on those files — presumed safe per the flag-reading guidance. No spec/schema/dist/changeset surface is in the diff, so the AdCP governance gates do not apply.
3216b0d to
0c14d9f
Compare
The base branch was changed.
135037f to
1e0116c
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean fail-closed auth-hardening pass, no blocking or medium findings.
Checked:
- Diff scope is server/** only (routes, middleware, auth, db, tests). No protocol surface (
static/schemas/source/**,docs/reference/**,dist/**, task/compliance assets) is touched, so no changeset is required — the app/auth exemption applies. - Fail-closed conversion is correct: provider unavailability throws 503 rather than masquerading as an invalid credential; no cookie fallback when a bearer is presented.
- Full call-site coverage for the now-throwing
resolveCallerOrgId; Express 5 auto-forwards the new token-route error middleware. - Bounded timeouts on all WorkOS/fetch/DB paths; leak-safe connection lifecycle in the largest new file
api-key-management-db.ts.
high_risk is true only via (modified) matches under server/src/routes/**, but the reviewer surfaced no medium-or-higher concerns on those files, so the modifications are presumed contract-preserving (no escalation on the flag alone). No no-auto-approve team match. gated_paths is false, so the REVIEW_REQUIRED status does not force a gate. No prior-escalation stickiness (prior decision was approve).
Decision table: rows 1–8 do not fire (no critical/high, gated_paths false, no deletions, no medium findings, no team gate) → row 9 approve.
API-key authentication and management bind to the exact authenticated credential and explicit organization. Management requires fresh direct active owner/admin membership independent of credential-grant storage. Invalid presented Bearers return terminal 401; unavailable verification returns terminal 503. Strict identifiers and fixed-origin URL construction constrain provider access.
Create/revoke and new API-key widget delegation stop before provider mutation. Inventory remains buffered until post-read external and local authority checks pass. Local locks do not make WorkOS membership atomic; there is no mutation retry.
Exact composition:
1e0116c28de9cbcf9e7f09b1dc7b83618f75ef91; tree:0984916e228bf3d0b504a8f824854855044bae85.32f25d968b73c2bbabd68d7ed9fbf2dfd127eb4f, the verified fix(auth): hydrate primary authorization snapshots per request #7462 squash landing (tree8bdd72b68c7e0e8d01240d57b0c7542137db5edb).3216b0d61dca45211f4a500a6508e3bdcd0b3113..2d96ea6e2d1bc32cd7533c4596daef4ef39f3e64; no historical fix(auth): hydrate primary authorization snapshots per request #7462 series was replayed. Original author/committer metadata and full commit message are preserved.Local qualification on this exact tree:
git commit: exit 0; 1,063 root tests in 71 files; 10,654 server tests in 641 files, 31 unchanged skips; all three loopback tests, repository guards and both typechecks passed. Qualification used isolated Node v24.21.0 with IPv6 loopback enabled and a successful connection probe from the start. No hook bypass or test exclusion was introduced.Residual limits: external changes after the last provider observation and unobservable change-and-revert transitions remain outside the observed-read contract. Already-issued widget tokens are not revoked by stopping new issuance. Mutation re-enablement needs a reviewed same-authority mechanism. Canonical organization discovery and later explicit-context work remain separate; this does not close the whole #6827 cross-surface criterion.
Main-targeted CI and review: All 32 current check runs and the IPR status passed on this exact head against the qualified main. Build and all test shards, repository CodeQL, repository security aggregate, managed Code Quality, migration checks and IPR are green. CI integration totals 2,354 passed with eight unchanged skips; the webhook file passed all 15 tests. Ready Ladon run executed Reviewer and Arbiter successfully on this exact head; native aao-secretariat APPROVED is bound to the same commit. The single identified Draft-only Ladon skip was superseded by Ready success. Zero unresolved threads and CLEAN mergeability; synthetic merge
5d23e7ed0fb2bd0fee3f62765a61438a25a6df7bhas parents qualified main + this head and exactly the reviewed tree. These gates are rechecked immediately before the guarded squash merge.Required order remains #7462 → #7461 → later dependencies. Auto-merge is off. Merge requires all fresh exact-head checks green, executed Ladon Reviewer/Arbiter and native aao-secretariat APPROVED, zero unresolved threads, CLEAN mergeability, and a synthetic merge tree equal to the reviewed tree.